Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SwipeableDrawer] Remove internal accesses in the tests #15469

Merged
merged 11 commits into from
May 8, 2019
Merged

[SwipeableDrawer] Remove internal accesses in the tests #15469

merged 11 commits into from
May 8, 2019

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding added new feature New feature or request component: SwipeableDrawer The React component. labels Apr 23, 2019
@joshwooding joshwooding mentioned this pull request Apr 23, 2019
Closed
29 tasks
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 23, 2019 •
edited
Loading

@material-ui/core: parsed: -0.42% 😍, gzip: -0.23% 😍

Details of bundle changes.

Comparing: 4516caf...de92e06

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.42% -0.23% 311,238 309,942 84,649 84,457
@material-ui/core/Paper 0.00% -0.01% 67,759 67,759 20,137 20,135
@material-ui/core/Paper.esm 0.00% +0.01% 🔺 61,056 61,056 18,942 18,943
@material-ui/core/Popper 0.00% +0.01% 🔺 28,590 28,590 10,289 10,290
@material-ui/core/Textarea 0.00% -0.08% 5,513 5,513 2,379 2,377
@material-ui/core/TrapFocus 0.00% +0.06% 🔺 3,780 3,780 1,576 1,577
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,958 15,958 5,781 5,781
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% 0.00% 140,251 140,251 42,642 42,642
@material-ui/styles 0.00% +0.03% 🔺 51,272 51,272 15,165 15,169
@material-ui/system 0.00% 0.00% 11,765 11,765 3,924 3,924
Button 0.00% -0.01% 85,706 85,706 25,646 25,643
Modal 0.00% +0.01% 🔺 20,367 20,367 6,693 6,694
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,531 51,531 11,369 11,369
docs.main -0.21% -0.10% 652,054 650,705 203,901 203,697
packages/material-ui/build/umd/material-ui.production.min.js -0.44% -0.25% 292,694 291,408 82,629 82,422

Generated by 🚫 dangerJS against de92e06

@joshwooding joshwooding force-pushed the swipeabledrawer-function-component branch 2 times, most recently from b1b716e to 88afeb0 Compare April 26, 2019 23:16
@joshwooding joshwooding force-pushed the swipeabledrawer-function-component branch 2 times, most recently from e06a006 to c800aa0 Compare April 29, 2019 23:23
@joshwooding
Copy link
Member Author

Removed setting styles via useEffect. It's a nice to have but the work needed to get it working might be substantial

@joshwooding joshwooding force-pushed the swipeabledrawer-function-component branch from c800aa0 to e14a553 Compare May 3, 2019 21:59
joshwooding
joshwooding commented May 6, 2019
View reviewed changes
packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js Outdated
@@ -416,7 +416,6 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(props, ref) {
}}
anchor={anchor}
transitionDuration={transitionDuration}
hideBackdrop={hideBackdrop}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it looks like this is passed through in the old version but when variant="permanent" it gets passed to a div.

@joshwooding joshwooding force-pushed the swipeabledrawer-function-component branch from dc7fabe to 6a49299 Compare May 6, 2019 22:45
@joshwooding
Copy link
Member Author

joshwooding commented May 6, 2019 •
edited
Loading

Looks like the last test failing is an actual failure. useCallback captures a snapshot of open so it doesn't cancel the closing.

Edit: Fixed using a ref

@joshwooding joshwooding marked this pull request as ready for review May 6, 2019 23:18
@mui-pr-bot
Copy link

mui-pr-bot commented May 7, 2019 •
edited
Loading

Details of bundle changes.

Comparing: 1555e52...fa4d8c0

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.03% 🔺 -0.01% 316,779 316,868 86,501 86,494
@material-ui/core/Paper 0.00% 0.00% 67,859 67,859 20,165 20,165
@material-ui/core/Paper.esm 0.00% 0.00% 61,152 61,152 18,956 18,956
@material-ui/core/Popper 0.00% 0.00% 28,738 28,738 10,349 10,349
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,384 2,384
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,580 1,580
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,779 5,779
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% 0.00% 140,261 140,261 42,708 42,708
@material-ui/styles 0.00% 0.00% 51,344 51,344 15,182 15,182
@material-ui/system 0.00% 0.00% 14,463 14,463 4,180 4,180
Button 0.00% 0.00% 85,558 85,558 25,633 25,633
Modal 0.00% 0.00% 20,342 20,342 6,696 6,696
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,200 51,200 11,348 11,348
docs.main +0.01% 🔺 0.00% 653,992 654,081 205,146 205,147
packages/material-ui/build/umd/material-ui.production.min.js +0.03% 🔺 -0.01% 295,677 295,766 83,867 83,861

Generated by 🚫 dangerJS against fa4d8c0

@joshwooding
Copy link
Member Author

Not entirely sure on how to continue with this :/ I have no way of determining why it fails on Chrome v41 on OS X

@oliviertassinari
Copy link
Member

@joshwooding Let me know if you want some creds to access BrowserStack, in the meantime. Let's see if I can help here :).

@oliviertassinari oliviertassinari self-assigned this May 8, 2019
@oliviertassinari oliviertassinari force-pushed the swipeabledrawer-function-component branch from 529399e to a02c4c3 Compare May 8, 2019 12:24
@oliviertassinari oliviertassinari force-pushed the swipeabledrawer-function-component branch from a02c4c3 to be73565 Compare May 8, 2019 12:25
@oliviertassinari
Copy link
Member

oliviertassinari commented May 8, 2019 •
edited
Loading

@joshwooding I have reverted the component changes to focus on the test changes. The build is green now. I think that moving away from internal tests in already very valuable.

Once the test changes are merged, we can move forward with the implementation :).

@oliviertassinari oliviertassinari changed the title [SwipeableDrawer] Convert to function component [SwipeableDrawer] Remove internal accesses in the tests May 8, 2019
@oliviertassinari oliviertassinari removed their assignment May 8, 2019
@oliviertassinari oliviertassinari merged commit 8d5d098 into mui:next May 8, 2019
@joshwooding joshwooding deleted the swipeabledrawer-function-component branch May 8, 2019 21:49
@oliviertassinari
Copy link
Member

@joshwooding Given that we already have the context fresh in our memory for the migration of this component, what do you think about completing the migration before starting another effort? I was about to say that we could delay it but we will lose some context if we handle it in the future.

@joshwooding
Copy link
Member Author

@oliviertassinari Sure :) As always the tests are the hardest part of nearly all of the migrations. Hopefully now we have done the tests the actually migration should be easy 🤞

@oliviertassinari oliviertassinari added the component: drawer This is the name of the generic UI component, not the React module! label Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
component: drawer This is the name of the generic UI component, not the React module! component: SwipeableDrawer The React component. new feature New feature or request test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshwooding @mui-pr-bot @oliviertassinari